fix(txpool): restore age-based future eviction - #686
Conversation
|
Not super important for now |
n13
left a comment
There was a problem hiding this comment.
Reviewed with the specific goal of confirming this restores the original upstream future-queue logic rather than writing a new age-based policy.
Upstream comparison — exact match confirmed
I extracted enforce_limits from four revisions and diffed them:
| Revision | Result vs. this PR |
|---|---|
472423e3 — vendored sc-transaction-pool 44.1.0, post-cargo fmt |
byte-identical |
b51fa56d^ — last commit before the tip/priority change |
byte-identical |
4d3d6d45 — the chain-private commit this ports |
byte-identical |
2b4eddf4 — raw vendor drop, pre-cargo fmt |
identical modulo rustfmt (Ordering::Equal => brace style, break; semicolons) |
So the future-eviction fold is not a re-derivation — it is the upstream code, character for character, in the repo's own formatting. The revert is complete: nothing of b51fa56d's priority-first fold survives, and the intermediate future-queue fixes (98105919, 601b567b, 91e3a7bb) are untouched.
Correctness of the security argument
Confirmed in-tree rather than taken on faith:
runtime/src/lib.rs:192includespallet_transaction_payment::ChargeTransactionPayment, so validation priority is tip-derived, and a nonce-gapped transaction never dispatches — the advertised tip is never charged. Priority on a future transaction is therefore free to fabricate.client/transaction-pool/src/graph/validated_pool.rsbans everything eviction removes (self.rotator.ban(&Instant::now(), removed.iter().copied())).
Together those make priority-first future eviction strictly worse than age-based: an attacker pins the queue at zero cost and gets every honest future transaction banned for ban_time. Age-based eviction doesn't make future-queue flooding free — an attacker can still push out older entries — but it costs them continuous spam and ages out their own entries too. Right call, and the PR description's framing of the tradeoff is accurate.
Checks
cargo test -p sc-transaction-pool --lib graph::base_pool— 19 passed, 0 failed, includingfuture_limit_enforcement_evicts_oldest_first.cargo +nightly fmt -p sc-transaction-pool -- --check— clean.use std::cmp::Ordering(line 23) is still live: the ready-queue fold at lines 509–517 uses it. No dead import, so no-D warningsrisk.
Notes (non-blocking)
-
base_pool.rs:487-488— doc comment now diverges from upstream. Upstream says "lowest priority first or those that occupy the pool for the longest time in case priority is the same," which describes only the ready queue and was never true of the future queue. The replacement is more accurate. It's the one intentional deviation and it adds a small merge-conflict surface on the next vendor refresh; I'd keep it — accuracy wins. -
Consider anchoring the rationale in the code. This policy has now flipped twice (
b51fa56d→ this PR), and after the revert nothing in the file records why age-based is deliberate. The next reader sees the same "age-only eviction looks exploitable" shape that motivatedb51fa56d. A one-line reference to Immunefi #90496 would prevent a repeat — best placed in the doc comment above (already divergent from upstream) rather than inside the fold, so the restored block stays byte-identical to upstream. -
base_pool.rs:1465— test comment reads backwards. "Priority must not allow an old future transaction to remain pinned in the pool" is hard to parse against the assertion that the old tx is evicted. Something like "high priority must not exempt an older future tx from age-based eviction" states what's being tested. Trivial.
Verdict: LGTM
The eviction logic is provably the pre-tip-change upstream code, the revert is complete and self-consistent, and the test correctly discriminates age-based from priority-based behavior (the older transaction is the higher-priority one, so it only passes under age-first). Only the three cosmetic notes above; none block merge.
Side note: This only affects the future queue, which is really only a temporary space to hold transactions that arrive out of order
An attacker could fill up future queue with high tip amounts and then basically cripple the future queue with that, so age based eviction is simpler.
But overall most transactions will end up in the ready queue and will be totally unaffected by any of this.
Tip based priority is easier to tamper with though so I still think we should do this.
Port of chain-private #39, which did not survive the private→public merge-back.
Priority-first eviction of the future queue is unsafe here: tips on nonce-gap transactions are not charged unless they execute, so an attacker can pin the queue with fake high tips and evict victims. Restore oldest-first eviction (Immunefi #90496).
future_limit_enforcement_evicts_oldest_firstpasses.